-
Couldn't load subscription status.
- Fork 23
🐛 Handle early cleanup bug with fix-all flows #957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
28800dd to
505803c
Compare
|
Warning Rate limit exceeded@ibolton336 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (12)
WalkthroughAdds a new SolutionWorkflowOrchestrator class and rewires the getSolution command to delegate to it. Extends ExtensionState and shared ExtensionData with isProcessingQueuedMessages, currentQueueManager, and pendingInteractionsMap. Introduces queue-management behaviors and logging in MessageQueueManager, handleFileResponse, and modified-file handlers, including a non-blocking interaction promise with timeout. UI components and state context are updated to reflect the new processing flag and to show progress/backdrop while queued messages are processed. Exports executeDeferredWorkflowDisposal. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
vscode/core/src/utilities/ModifiedFiles/handleFileResponse.ts (3)
147-148: Use nullish coalescing so empty-string edits aren’t lost
||treats""as falsy and will ignore an intentional empty file update. Use??to only fall back onundefined/null.- const fileContent = content || fileValue.content; + const fileContent = content ?? fileValue.content;
240-243: Don’t log full ModifiedFile payloads (large/sensitive); log metadata onlyLogging
valuecan dump entire file contents. Emit path/flags/length instead.- logger.debug(`[handleFileResponse] Found fileMessage for token ${messageToken}:`, { - found: !!fileMessage, - value: fileMessage?.value, - }); + logger.debug(`[handleFileResponse] Found fileMessage for token ${messageToken}`, { + found: !!fileMessage, + path: + (fileMessage?.value as ModifiedFileMessageValue | undefined)?.path, + isNew: + (fileMessage?.value as ModifiedFileMessageValue | undefined)?.isNew, + isDeleted: + (fileMessage?.value as ModifiedFileMessageValue | undefined)?.isDeleted, + contentLength: typeof (fileMessage?.value as ModifiedFileMessageValue | undefined)?.content === "string" + ? (fileMessage!.value as ModifiedFileMessageValue).content.length + : undefined, + });
19-29: Cross‑platform path handling
substring(...lastIndexOf("/"))breaks on Windows. Usepath.dirname.+import * as path from "path"; @@ - const directoryPath = filePath.substring(0, filePath.lastIndexOf("/")); - if (directoryPath) { - const directoryUri = vscode.Uri.file(directoryPath); - try { - await vscode.workspace.fs.createDirectory(directoryUri); - } catch (dirError) { - state.logger - .child({ component: "handleFileResponse.createNewFile" }) - .error(`Failed to create directory at ${directoryPath}:`, dirError); - } - } + const directoryUri = vscode.Uri.file(path.dirname(filePath)); + try { + await vscode.workspace.fs.createDirectory(directoryUri); + } catch (dirError) { + state.logger + .child({ component: "handleFileResponse.createNewFile" }) + .error(`Failed to create directory for ${filePath}:`, dirError); + }vscode/core/src/utilities/ModifiedFiles/queueManager.ts (1)
16-22: Missing “drain” signal: orchestrator can get stuck after deferred cleanup with no interactions.If cleanupAfterExecution defers and no user interactions occur, nothing triggers final cleanup when the queue later drains. Provide an onDrain callback to notify the orchestrator when the queue empties without waiting for user input.
export class MessageQueueManager { @@ - constructor( + constructor( private state: ExtensionState, private workflow: KaiInteractiveWorkflow, private modifiedFilesPromises: Array<Promise<void>>, private processedTokens: Set<string>, private pendingInteractions: Map<string, (response: any) => void>, + private onDrain?: () => void, ) { @@ - this.logger.info(`Queue processing complete, ${this.messageQueue.length} messages remaining`); + this.logger.info(`Queue processing complete, ${this.messageQueue.length} messages remaining`); + // Notify when fully drained and not blocked by user interaction + if (!this.state.data.isWaitingForUserInteraction && this.messageQueue.length === 0) { + try { + this.onDrain?.(); + } catch (e) { + this.logger.error("Error in onDrain callback:", e); + } + }This pairs with orchestrator changes to finalize once workflowRunCompleted and the queue drains. See proposed orchestrator diff in its file comment.
Also applies to: 145-146
🧹 Nitpick comments (13)
vscode/core/src/utilities/ModifiedFiles/handleModifiedFile.ts (1)
184-186: Replaceconsole.*with structured loggerUse
state.logger.child({ component: "handleModifiedFileMessage" })for uniform logs and levels.@@ - console.error( - `ModifiedFile interaction timeout for ${filePath} (${msg.id}) - auto-resolving to prevent stuck state`, - ); + const logger = state.logger.child({ component: "handleModifiedFileMessage" }); + logger.warn("ModifiedFile interaction timeout - auto-resolving", { filePath, messageId: msg.id }); @@ - console.error(`Error in ModifiedFile resolver for messageId: ${msg.id}:`, error); + const logger = state.logger.child({ component: "handleModifiedFileMessage" }); + logger.error("Error in ModifiedFile resolver", { messageId: msg.id, error }); @@ - console.error(`Error in handleModifiedFileMessage for ${filePath}:`, err); + state.logger.error("Error in handleModifiedFileMessage", { filePath, error: err }); @@ - console.error(`Error during cleanup for ${filePath}:`, cleanupError); + state.logger.error("Error during cleanup", { filePath, error: cleanupError });Additionally, initialize the child logger once near the top of the function to avoid re-creating it.
Also applies to: 221-221, 235-235, 252-252
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
203-206: Nit: remove unused dependency to avoid re-renders
isFetchingSolutionisn’t used insiderenderChatMessages.- }, [chatMessages, isFetchingSolution, isAnalyzing, triggerScrollOnUserAction]); + }, [chatMessages, isAnalyzing, triggerScrollOnUserAction]);vscode/core/src/commands.ts (1)
195-205: Also resetisProcessingQueuedMessagesin the manual reset commandPrevents stuck “Processing…” UI after failures.
state.mutateData((draft) => { draft.isFetchingSolution = false; if (draft.solutionState === "started") { draft.solutionState = "failedOnSending"; } draft.isWaitingForUserInteraction = false; + draft.isProcessingQueuedMessages = false; });vscode/core/src/utilities/ModifiedFiles/queueManager.ts (4)
23-28: Initialize logger before starting background timer.Minor race: setInterval is scheduled before this.logger is assigned. It’s unlikely to tick first, but invert the order for safety.
- // Start background processor that runs continuously - this.startBackgroundProcessor(); - this.logger = state.logger.child({ + this.logger = state.logger.child({ component: "MessageQueueManager", }); + // Start background processor that runs continuously + this.startBackgroundProcessor();
35-37: Reduce log noise; use debug for per-message logs.Per-message info logs will flood logs at scale. Keep start/end at info; switch enqueue/each-message to debug.
- this.logger.debug( + this.logger.debug( `Message enqueued: ${message.type}, id: ${message.id}, queue length: ${this.messageQueue.length}`, ); @@ - this.logger.info(`Starting queue processing, ${this.messageQueue.length} messages in queue`); + this.logger.info(`Starting queue processing, ${this.messageQueue.length} messages in queue`); @@ - this.logger.info( + this.logger.debug( `Processing message: ${msg.type}, id: ${msg.id}, remaining in queue: ${this.messageQueue.length}`, ); @@ - this.logger.info(`Queue processing complete, ${this.messageQueue.length} messages remaining`); + this.logger.info(`Queue processing complete, ${this.messageQueue.length} messages remaining`);Also applies to: 107-107, 115-117, 145-145
119-131: Avoid dynamic import inside the processing loop.Import once outside the loop to reduce overhead (module cache helps, but hoisting is cleaner).
try { - // Process messages one at a time from the front of the queue + // Import once + const { processMessageByType } = await import("./processMessage"); + // Process messages one at a time from the front of the queue while (this.messageQueue.length > 0 && !this.state.data.isWaitingForUserInteraction) { @@ - // Call the core processing logic directly - const { processMessageByType } = await import("./processMessage"); await processMessageByType(
189-204: Optionally set UI processing flag on resume after interaction.When resuming with a non-empty queue, consider surfacing progress to the UI.
state.mutateData((draft) => { draft.isWaitingForUserInteraction = false; + if (queueManager.getQueueLength() > 0) { + draft.isProcessingQueuedMessages = true; + } });webview-ui/src/context/ExtensionStateContext.tsx (2)
48-50: Prefer nullish coalescing to preserve explicit false values.Use ?? instead of || for booleans coming from external data.
- isWaitingForUserInteraction: windowData.isWaitingForUserInteraction || false, - isProcessingQueuedMessages: windowData.isProcessingQueuedMessages || false, + isWaitingForUserInteraction: windowData.isWaitingForUserInteraction ?? false, + isProcessingQueuedMessages: windowData.isProcessingQueuedMessages ?? false, @@ - isWaitingForUserInteraction: event.data.isWaitingForUserInteraction || false, - isProcessingQueuedMessages: event.data.isProcessingQueuedMessages || false, + isWaitingForUserInteraction: event.data.isWaitingForUserInteraction ?? false, + isProcessingQueuedMessages: event.data.isProcessingQueuedMessages ?? false,Also applies to: 86-87
71-97: Don’t re-register window message listener on state changes.The effect re-runs on chat length/waiting flag changes, causing unnecessary add/remove. Register once on mount.
- useEffect(() => { + useEffect(() => { const handleMessage = (event: MessageEvent<ExtensionData>) => { @@ return () => { window.removeEventListener("message", handleMessage); }; - }, [state.chatMessages.length, state.isWaitingForUserInteraction]); + }, []); // register onceIf you need latest state in the handler, derive it from event.data (as you already do), so a stable listener is fine.
vscode/core/src/solutionWorkflowOrchestrator.ts (4)
20-22: Type the workflow field.Avoid any; use KaiInteractiveWorkflow for safer refactoring.
- private workflow: any; + private workflow!: import("@editor-extensions/agentic").KaiInteractiveWorkflow;
288-301: Brittle error detection by string match.Checking for “Invalid string length” is fragile. Prefer error.name/type (RangeError) and/or a predicate helper.
- const isStringLengthError = errorMessage.includes("Invalid string length"); + const isStringLengthError = + (err instanceof RangeError && err.message.includes("Invalid string length")) || + errorMessage.includes("Invalid string length"); // fallback
230-235: Hardcoded programming language.If available in incidents or settings, derive programmingLanguage instead of hardcoding "Java".
const programmingLanguage = this.incidents[0]?.language ?? this.state.profiles?.find(p => p.name === profileName)?.language ?? "Java";
441-450: Return type for validation could be discriminated for clarity.Using strings to branch on UI (warning vs error) is brittle. Consider {valid:false, code:'already-fetching'|'genai-disabled'|...}.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
shared/src/types/types.ts(1 hunks)vscode/core/src/commands.ts(4 hunks)vscode/core/src/extension.ts(1 hunks)vscode/core/src/extensionState.ts(2 hunks)vscode/core/src/solutionWorkflowOrchestrator.ts(1 hunks)vscode/core/src/utilities/ModifiedFiles/handleFileResponse.ts(1 hunks)vscode/core/src/utilities/ModifiedFiles/handleModifiedFile.ts(1 hunks)vscode/core/src/utilities/ModifiedFiles/queueManager.ts(4 hunks)webview-ui/src/components/AnalysisPage/AnalysisPage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(2 hunks)webview-ui/src/context/ExtensionStateContext.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-30T13:30:21.839Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#874
File: vscode/src/utilities/ModifiedFiles/handleFileResponse.ts:238-248
Timestamp: 2025-09-30T13:30:21.839Z
Learning: In the vscode extension's ModifiedFile handling (vscode/src/utilities/ModifiedFiles/handleFileResponse.ts), not all ModifiedFile messages have a userInteraction field associated with them. The hasUserInteraction check is used to conditionally add the response field only when there is an actual userInteraction, because only those cases have a pending workflow promise that needs to be resolved.
Applied to files:
vscode/core/src/utilities/ModifiedFiles/handleFileResponse.tsvscode/core/src/utilities/ModifiedFiles/handleModifiedFile.ts
📚 Learning: 2025-08-12T02:13:55.897Z
Learnt from: djzager
PR: konveyor/editor-extensions#678
File: vscode/src/commands.ts:135-139
Timestamp: 2025-08-12T02:13:55.897Z
Learning: In vscode/src/commands.ts, the solutionState is conditionally reset to "none" after successful getSolution completion, but only when !state.isWaitingForUserInteraction. This prevents permanent blocking of future getSolution calls while still maintaining proper concurrency control during active workflows.
Applied to files:
vscode/core/src/commands.ts
🧬 Code graph analysis (7)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
webview-ui/src/hooks/useScrollManagement.ts (1)
useScrollManagement(5-259)
vscode/core/src/utilities/ModifiedFiles/handleFileResponse.ts (1)
vscode/core/src/client/tracer.ts (1)
logger(60-65)
vscode/core/src/utilities/ModifiedFiles/handleModifiedFile.ts (1)
vscode/core/src/utilities/ModifiedFiles/queueManager.ts (1)
handleUserInteractionComplete(185-205)
vscode/core/src/solutionWorkflowOrchestrator.ts (8)
vscode/core/src/utilities/ModifiedFiles/queueManager.ts (1)
MessageQueueManager(10-179)vscode/core/src/extensionState.ts (1)
ExtensionState(20-56)shared/src/types/types.ts (2)
EnhancedIncident(51-64)Scope(78-80)vscode/core/src/utilities/configuration.ts (1)
getConfigAgentMode(73-74)agentic/src/types.ts (1)
KaiWorkflowMessage(58-64)vscode/core/src/utilities/ModifiedFiles/processMessage.ts (1)
processMessage(132-147)vscode/core/src/commands.ts (2)
executeDeferredWorkflowDisposal(55-61)executeExtensionCommand(48-50)agentic/src/workflows/interactiveWorkflow.ts (1)
KaiInteractiveWorkflowInput(33-37)
vscode/core/src/extensionState.ts (1)
vscode/core/src/utilities/ModifiedFiles/queueManager.ts (1)
MessageQueueManager(10-179)
vscode/core/src/utilities/ModifiedFiles/queueManager.ts (1)
tests/e2e/utilities/logger.ts (1)
error(41-43)
vscode/core/src/commands.ts (2)
vscode/core/src/extensionState.ts (1)
ExtensionState(20-56)vscode/core/src/solutionWorkflowOrchestrator.ts (1)
SolutionWorkflowOrchestrator(18-519)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test E2E
🔇 Additional comments (11)
vscode/core/src/utilities/ModifiedFiles/handleFileResponse.ts (2)
116-127: Good: stale pending-interaction auto-resolution on missing tokenEarly exit with resolver avoids hangs when the message was cleared. LGTM.
261-267: Correct: only attachresponsewhen a userInteraction existsPrevents resolving non-interactive ModifiedFile messages. LGTM. Based on learnings
vscode/core/src/utilities/ModifiedFiles/handleModifiedFile.ts (1)
175-181: Non‑blocking pending‑interaction is the right callNot awaiting the promise avoids queue deadlocks while still enabling cleanup. LGTM.
Also applies to: 229-233
shared/src/types/types.ts (2)
136-137: Good addition: public processing flag
isProcessingQueuedMessagesenables accurate UI progress state. LGTM.
266-281: Type parity: ensure single source‑of‑truth for KaiUserInteractionThis interface also exists in @editor-extensions/agentic. Mismatches (e.g.,
"modifiedFile"union member) will cause compile/runtime drift.Consider re‑exporting the agentic type here or aliasing one to the other to avoid divergence.
vscode/core/src/extension.ts (1)
98-99: Init default is correct; verify all failure paths reset this flagOrchestrator sets it true when queue has work and resets it in final cleanup. Ensure error paths (e.g., workflow error/abort) also set
isProcessingQueuedMessages = falseso UI backdrops don’t stick.webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
127-136: Unified processing state looks goodDeriving
isProcessingand feeding the scroll hook plus header indicator is consistent with the new queue flow. LGTM.Also applies to: 219-221
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
377-389: Backdrop/title integration for queue processing is correctAccurately reflects queued processing vs waiting for user/solution. LGTM.
vscode/core/src/extensionState.ts (1)
18-19: State surface extension is appropriate
currentQueueManagerandpendingInteractionsMapbelong on state for cross-module coordination. LGTM.Also applies to: 54-56
vscode/core/src/commands.ts (2)
55-61: Exporting deferred disposal helper is fine; check import sitesConfirm all import paths updated (orchestrator and any tests) and no circular deps created.
157-159: Orchestrator delegation for getSolutionGood modularization and central lifecycle handling. LGTM.
cleanup getSolution flow Add e2e tests to handle fix all Handle agent follow up messages gracefully in orchestrator Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
|
@pranavgaikwad @djzager this one is ready for a look if either of you has a spare cycle. |
Bug: Fix all issues in non-agent mode hangs
Summary by CodeRabbit
New Features
Improvements